changes in Interfaces base (closes #2320) #2387

djarecka · 2018-01-21T04:35:40Z

changes in interface base listed in #2320:

_check_version_requirements does not check the max_version, this block has wrong indentation
I was a bit confused about BaseFile and File, etc., do we really need both? Also I believe that we're assuming that BaseUnicode uses the fast validator (https://github.com/oesteban/nipype/blob/ref/interface-base/nipype/interfaces/base/traits_extension.py#L98), but from traits documentation I understood that Unicode (not BaseUnicode) uses the C-level fast validator (http://docs.enthought.com/traits/traits_api_reference/trait_types.html?highlight=baseunicode#traits.trait_types.BaseUnicode. Am I missing something?
couldn't find a place where we're using _requires_warn
the same about _hash_infile
is __pretty__ still working?
is it for this package: http://ipython.readthedocs.io/en/stable/api/generated/IPython.lib.pretty.html#extending ?
they suggest _repr_pretty_
Check on this synchronize
Check the logic of this

…work)

effigies · 2018-01-21T13:33:57Z

Is this intended for 1.0, 1.0 maintenance, or 2.0?

djarecka · 2018-01-22T03:21:41Z

@effigies this are for 1.0, but don't know if we have to wait to include everything before release. Was planning to do it last week, but didn't happen..

Notice, that these are independent points that were suggested by me or @satra during #2303 revision. If you or @satra find time to review it we can merge before finishing all points (I'm trying to keep them in separate commits) and I can open a new PR.
that would eliminate a bug in _check_version_requirements.

djarecka · 2018-01-22T04:03:16Z

do you know anyone using MpiCommandLine? should I remove it?

effigies · 2018-01-22T04:15:23Z

Okay, I can have a look.

As for MpiCommandLine, I would generally prefer not to remove functionality that someone may be using without a deprecation cycle. i.e. We should have had a few releases where we raised warnings that it was going to go away, which would give people time to complain or find another approach before we actually pull the rug out from under them. So unless there's a really compelling reason to remove it, I would leave it. If people think it's not worth supporting, we can deprecate it so it can be removed later.

…t use a afast validator, so i renamed BaseFile(Directory) to File(Directory) and removed extra classes

djarecka · 2018-01-22T04:29:31Z

@effigies ok, I removed MpiCommandLine from the list.

Note that my last commits renamed MultiPath (as suggested by @satra some time ago), and removed BaseFile/BaseDirectory (because they were not used and didn't differ from File/Directory). Hope that's ok, but if you think that would cause problems, I can revert

effigies

Commenting on the first 5 points (perhaps not in order).

I would be inclined to leave *MultiPath as it is for 1.0. External users depend on it. We can change it in 2.0. If you think it would be good to have *MultiObject for 1.0, I would simply do:

InputMultiObject = InputMultiPath
OutputMultiObject = OutputMultiPath

This would allow users to begin switching to the new syntax before 2.0, without modifying the class hierarchy.

I'll get to the rest shortly, but wanted to get you some feedback right now.

effigies · 2018-01-22T16:47:56Z

nipype/interfaces/base/core.py

+        check = dict(max_ver=lambda t: t is not None)
+        names = trait_object.trait_names(**check)
+        if names and self.version:
+            version = LooseVersion(str(self.version))


I'm not sure I see a semantic difference in 955b3ad. I see:

if names and self.version: ... # Check min_ver check = dict(...) names = trait_object.trait_names(**check) ... # Check max_ver

Which becomes:

if names and self.version: ... # Check min_ver check = dict(...) names = trait_object.trait_names(**check) if names and self.version: ... # Check max_ver

Am I missing something?

answered lower in the discussion

effigies · 2018-01-22T17:00:00Z

nipype/interfaces/base/specs.py

@@ -119,20 +119,6 @@ def _xor_warn(self, obj, name, old, new):
                           'which is already set') % (name, trait_name)
                    raise IOError(msg)

-    def _requires_warn(self, obj, name, old, new):


This used to be called from _generate_handlers.

nipype/nipype/interfaces/base.py

Lines 389 to 390 in 5495816

for elem in requires:

self.on_trait_change(self._requires_warn, elem)

I was unable to quickly find the PR it was removed in, so it's hard to tell whether its removal was intentional or a mistake. As an unused private methods, I'm okay with getting rid of it, but @satra, if this should still be in _generate_handlers, let us know.

effigies · 2018-01-22T17:01:21Z

nipype/interfaces/base/traits_extension.py

@@ -54,7 +54,7 @@ class Str(Unicode):
 traits.DictStrStr = DictStrStr


-class BaseFile(BaseUnicode):
+class File(BaseUnicode):


I'm unclear on the original justification for this separation, but dropping BaseFile seems fine to me.

effigies · 2018-01-22T17:04:48Z

nipype/interfaces/base/specs.py

@@ -165,29 +151,6 @@ def _deprecated_warn(self, obj, name, old, new):
                            '%s' % trait_spec.new_name: new
                        })

-    def _hash_infile(self, adict, key):


Unused private method. Good to remove, as far as I'm concerned. Again, @satra, feel free to override if this looks like a mistake.

effigies · 2018-01-22T17:11:46Z

nipype/interfaces/base/support.py

-        """Support for the pretty module
-
-        pretty is included in ipython.externals for ipython > 0.10"""
+    def _repr_pretty_(self, p, cycle):


I'm okay with this, but we should verify that we can pretty print Bunches in IPython with this change. I don't know if the API changed beyond what method it looks for in your objects.

if it doesn't work quickly, I'd say remove the function altogether and create an issue to fix and re-add, rather than waste time on this today.

I've just checked that pretty from IPython.lib.pretty uses _repr_pretty_ method (and completely ignored when it was named __pretty__).
In my opinion it looks as "pretty" as simply printing Bunch, so I can either remove it or keep my name.

Let me know if this was design to work with different library.

It looks like what it does is prevents an infinite loop during formating. Could you test with a bunch that contains itself?

yes, you're right, for a bunch that contains itself it makes a difference! But only with the new name _repr_pretty_.
And you don't actually have to import pretty in ipython.

effigies · 2018-01-22T17:22:40Z

I would not refactor Bunch this close to release. That feels like a rabbit hole that should be entered on a leisurely time frame.

effigies · 2018-01-22T17:30:35Z

@djarecka Review complete. I'll look at the max_ver issue locally. I don't see how your change can be fixing it, but if the test fails before and works after, I'll allow it.

djarecka · 2018-01-22T17:31:51Z

@effigies ok, I'm just working actually on Bunch, but I can definitely set this as a new PR for future versions

satra · 2018-01-22T17:37:26Z

@djarecka - basefile and basedirectory provide different validation error messages than from traits. the code is not the same. please don't remove when the code does not match the traits code.

_requires_warn can be removed since we disabled that.
_hash_infile can be removed

there are many folks who rely on InputMultiPath for their external interfaces. MultiObject/Bunch should be left for the nipype 2.0 refactor.

djarecka · 2018-01-22T17:39:09Z

@effigies re max_ver:
notice how names are defined. They should be redefined for max version check.

The best way to see it doesn't work is to see what is the Exception that is really being raised here. After fixing it the test will fail

effigies · 2018-01-22T17:41:46Z

Ah, gotcha. Yeah, 👍 on max_ver fix.

djarecka · 2018-01-22T17:58:37Z

@satra @effigies:

I'll revert MultiObject and together with Bunch refactoring will wait till 2.0
I can revert my changes of basefile and basedirectory, but I don't understand your point @satra. I did NOT remove these classes, I removed File and Directory classes because IMO were the same as BaseFile and BaseDirectory. Since I didn't want to change the API, I simply renamed BaseFile to File and BaseDirectory to Directory.

Answering your question @effigies about the original justification: I'm not sure (@satra can correct me), but 2 classes File and BaseFile were created probably to provide an option to use either fast validator or not (see the difference between BaseUnicode and Unicode in traits doc. However the code that was changing validator in File and Directory classes were commented 2 years ago

satra · 2018-01-22T18:06:24Z

@djarecka - the reason for the classes have nothing to do with the validator, only the error message. trait's own error message is very cryptic for missing files/directories.

i'm fine with File/Directory being mapped directly to BaseFile/BaseDirectory

djarecka · 2018-01-22T18:10:34Z

yes, I only meant that the reason to have two pairs File/Directory and BaseFile/BaseDirectory was probably related to the validator options offered by traits.

It will be included in 2.0 This reverts commit 36ffda1.

djarecka · 2018-01-22T20:02:54Z

@satra @effigies if I understood all your comments correctly this should be ready for final review and merging if all tests pass

djarecka added 6 commits January 20, 2018 22:22

fixing test_input_version (showing that maximum version check doesnt …

6d65cbb

…work)

fixing _check_version_requirements, so it also checks maximal version

955b3ad

fixing and splitting test_input_version

40a0804

removing _requires_warn (not used)

3549566

removing _hash_infile (not used)

1794004

changing __pretty__ to _repr_pretty_

cc4c418

changing name MultiPath to MultiObject

36ffda1

File(Directory) class didnt dffer from BaseFile(Directory), i.e. didn…

ab8ae1a

…t use a afast validator, so i renamed BaseFile(Directory) to File(Directory) and removed extra classes

effigies reviewed Jan 22, 2018

View reviewed changes

effigies mentioned this pull request Jan 22, 2018

[MAINT] Cleaning / simplify Node #2325

Merged

6 tasks

Revert "changing name MultiPath to MultiObject"

252a745

It will be included in 2.0 This reverts commit 36ffda1.

effigies added this to the 1.0 milestone Jan 22, 2018

djarecka added 2 commits January 22, 2018 14:47

removing extra line from misc.py

11212a4

a small changes of comments about synchronize

c1ab001

effigies approved these changes Jan 22, 2018

View reviewed changes

effigies merged commit 3f5e185 into nipy:master Jan 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changes in Interfaces base (closes #2320) #2387

changes in Interfaces base (closes #2320) #2387

djarecka commented Jan 21, 2018 •

edited

Loading

effigies commented Jan 21, 2018

djarecka commented Jan 22, 2018

djarecka commented Jan 22, 2018

effigies commented Jan 22, 2018

djarecka commented Jan 22, 2018

effigies left a comment

effigies Jan 22, 2018

djarecka Jan 22, 2018

effigies Jan 22, 2018

effigies Jan 22, 2018

effigies Jan 22, 2018

effigies Jan 22, 2018

djarecka Jan 22, 2018 •

edited

Loading

effigies Jan 22, 2018

djarecka Jan 22, 2018

effigies commented Jan 22, 2018

effigies commented Jan 22, 2018

djarecka commented Jan 22, 2018

satra commented Jan 22, 2018

djarecka commented Jan 22, 2018 •

edited

Loading

effigies commented Jan 22, 2018

djarecka commented Jan 22, 2018 •

edited

Loading

satra commented Jan 22, 2018

djarecka commented Jan 22, 2018

djarecka commented Jan 22, 2018

	for elem in requires:
	self.on_trait_change(self._requires_warn, elem)

changes in Interfaces base (closes #2320) #2387

changes in Interfaces base (closes #2320) #2387

Conversation

djarecka commented Jan 21, 2018 • edited Loading

effigies commented Jan 21, 2018

djarecka commented Jan 22, 2018

djarecka commented Jan 22, 2018

effigies commented Jan 22, 2018

djarecka commented Jan 22, 2018

effigies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djarecka Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Jan 22, 2018

effigies commented Jan 22, 2018

djarecka commented Jan 22, 2018

satra commented Jan 22, 2018

djarecka commented Jan 22, 2018 • edited Loading

effigies commented Jan 22, 2018

djarecka commented Jan 22, 2018 • edited Loading

satra commented Jan 22, 2018

djarecka commented Jan 22, 2018

djarecka commented Jan 22, 2018

djarecka commented Jan 21, 2018 •

edited

Loading

djarecka Jan 22, 2018 •

edited

Loading

djarecka commented Jan 22, 2018 •

edited

Loading

djarecka commented Jan 22, 2018 •

edited

Loading